Skip to content

Conversation

@ChunkyProgrammer
Copy link
Contributor

@ChunkyProgrammer ChunkyProgrammer commented Jan 27, 2025

Before this PR:
video attachments for community posts would load an embedded video page (fetching video data, streaming links, etc.) and allowed playing the video within the community page. I imagine this could cause some rate limit issues + some unnecessary strain on some invidious instances (especially if there's multiple videos on the community page being loaded)
image

After this PR:
Thumbnail and some video data is shown as a link to the video page. Channel is also shown since I believe it's possible for channels to attach a video from channels that aren't there own.
image

@ChunkyProgrammer ChunkyProgrammer requested a review from a team as a code owner January 27, 2025 03:52
@ChunkyProgrammer ChunkyProgrammer requested review from unixfox and removed request for a team January 27, 2025 03:52
@ChunkyProgrammer ChunkyProgrammer force-pushed the dont-use-embed-video-on-com-page branch from 550dd39 to 9b6c28f Compare March 18, 2025 13:07
@ChunkyProgrammer ChunkyProgrammer force-pushed the dont-use-embed-video-on-com-page branch from 9b6c28f to 97a7e19 Compare June 17, 2025 13:17
Copy link
Member

@SamantazFox SamantazFox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I've made two small suggestions to reduce duplicated code.

@ChunkyProgrammer ChunkyProgrammer force-pushed the dont-use-embed-video-on-com-page branch from 45f532c to d5dfceb Compare September 13, 2025 15:23
@SamantazFox SamantazFox added the need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something label Sep 14, 2025
@SamantazFox SamantazFox added ready and removed need-testing This feature needs to be deployed and tested to see if it's working, and doesn't break something labels Oct 16, 2025
Copy link
Member

@Fijxu Fijxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the changes, it also requires #5567 to work.

Comment on lines +109 to +115
if thin_mode
html << %(<img loading="lazy" class="thumbnail" src="/vi/)
html << attachment["videoId"]
html << %(/mqdefault.jpg" alt="" />)
else
html << %(<div class="bottom-right-overlay">)
end
Copy link
Member

@Fijxu Fijxu Dec 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if thin_mode
html << %(<img loading="lazy" class="thumbnail" src="/vi/)
html << attachment["videoId"]
html << %(/mqdefault.jpg" alt="" />)
else
html << %(<div class="bottom-right-overlay">)
end
if thin_mode
html << %(<div class="bottom-right-overlay">)
else
html << %(<img loading="lazy" class="thumbnail" src="/vi/)
html << attachment["videoId"]
html << %(/mqdefault.jpg" alt="" />)
end

Logic issue, when thin_mode is true IT SHOULDN'T show a thumbnail.

@Fijxu Fijxu removed the ready label Dec 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants